Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sigprocmask #826

Merged
merged 1 commit into from
Jan 6, 2018
Merged

Add sigprocmask #826

merged 1 commit into from
Jan 6, 2018

Conversation

Thomasdezeeuw
Copy link
Contributor

No description provided.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. In addition to addressing my inline comments, make sure you add a CHANGELOG entry.

/// Examine and change blocked signals.
///
/// For more informations see the [`sigprocmask` man
/// pages](http://man7.org/linux/man-pages/man2/sigprocmask.2.html).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a portable function, so you should link to a portable man page.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/sigprocmask.html

}

#[test]
fn test_sigprocmask() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should grab SIGNAL_MTX here so that this doesn't race with any other tests.

.expect("expect to be able to block signals");

// And test it again, to make sure the change was effective.
sigprocmask(SigmaskHow::SIG_BLOCK, None, Some(&mut old_signal_set))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about emptying old_signal_set before this line?

@Thomasdezeeuw
Copy link
Contributor Author

Addressed all feedback, you might want to squash the commits since the second doesn't have a good commit message.

@Susurrus
Copy link
Contributor

Susurrus commented Jan 6, 2018

@Thomasdezeeuw Please go ahead and squash the commits on your end and force-push back to this branch.

@Thomasdezeeuw
Copy link
Contributor Author

@Susurrus can't you use the squash and merge button on GitHub? Force pushing to Github never seems to work for me.

@Susurrus
Copy link
Contributor

Susurrus commented Jan 6, 2018

We don't use GitHub to merge PRs, we rely on Bors, an external tool. It doesn't squash commits. So we rely on the PR authors to have their commits in order before we merge it.

@Thomasdezeeuw
Copy link
Contributor Author

@Susurrus I didn't know that, squashed the commits so this is ready to go.

@Susurrus
Copy link
Contributor

Susurrus commented Jan 6, 2018

It's somewhat mentioned in the CONYRIBUTING.MD document, but not very well. I've been meaning to update that actually as this comes up a lot.

@Thomasdezeeuw
Copy link
Contributor Author

It seems the build bot is failing, it times out after 5 minutes and then fails to kill it.

command timed out: 300 seconds without output running ['rustup', 'run', '1.20.0', 'cargo', 'test'], attempting to kill
SIGKILL failed to kill process
using fake rc=-1
program finished with exit code -1
remoteFailed: [Failure instance: Traceback from remote host -- exceptions.RuntimeError: SIGKILL failed to kill process
]


buildbot.process.remotecommand.RemoteException: exceptions.RuntimeError: SIGKILL failed to kill process
Traceback (most recent call last):
Failure: exceptions.RuntimeError: SIGKILL failed to kill process

And trying to digg in further fails to load (part of) the page.

@asomers
Copy link
Member

asomers commented Jan 6, 2018

It looks like my VM suddenly got about 10 times slower. Problems with the VM host? Meltdown/Spectra countermeasures? I don't know. I've bumped the timeouts and I'm restarting the builds.

@asomers
Copy link
Member

asomers commented Jan 6, 2018

bors r+

bors bot added a commit that referenced this pull request Jan 6, 2018
826: Add sigprocmask r=asomers a=Thomasdezeeuw
@bors
Copy link
Contributor

bors bot commented Jan 6, 2018

@bors bors bot merged commit efc9ac3 into nix-rust:master Jan 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants